Set up API client code generation #121

michpohl · 2024-10-23T05:30:24Z

This PR adds the main code generation setup to :tidalapi, as well as the generated files.

Please review commit by commit to have an easy overview, and just skip the commit adding the generated files 😄

Note for reviewers:

The code in this pull request is a MVP. Please point out anything you don't like, but the idea is to get this one merged and then improve what needs improving in subsequent PRs. So if you request changes it should be for things that absolutely can't go in like this.
Please also look at the additional info provided in the commit messages.

Note about the generated code:

I debated whether it should be in the src folder or not, but after some reading I decided yes, because:

We will commit the generated code
Generation will not happen as part of the normal build process
Generated code in the future will be changed and reviewed via PR.

How can you test this?

Check out the branch
Publish to MavenLocal
Add MavenLocal in settings.gradle.kts like this:

dependencyResolutionManagement {
    repositories {
        mavenCentral()
        }
}

Add a dependency to com.tidal.sdk.tidalapi:0.1.0 where you want to use it, and instantiate an api client like this:

 val client: TidalApiClient = TidalApiClient(credentialsProvider)

Play around with it.

Do you need to test it to review this?

At this stage, no imho. But if you like to, feel free!

How to/ test code generation?

As you currently need a binary of my fork of openapi-generator, ask me personally, if you want to play around with it.
But after this PR, we wil quickly move towards making this available via GH Actions, which will simplify this all.

michpohl · 2024-10-23T11:23:08Z

tidalapi/bin/generate-api-files.py

+    if result.returncode != 0:
+        logging.error("ktlint failed, but continuing with the rest of the script.")
+
+    target_line = "import com.tidal.sdk.tidalapi.generated.infrastructure.CollectionFormats.*"


Note that ktlint cannot fix these imports (* imports of types provided by us), so after it is run, we manually remove them anywhere they show up as we don't need them, but openapi-generator just puts it everywhere

michpohl · 2024-10-23T11:25:43Z

tidalapi/bin/generate-api-config.json

+
+  ],
+  "output": "./tidal-api-joined.json",
+  "openapi_generator_path": "tbd"


This would point to where the executable would be found. As I still work on providing it, it is tbd for now (which means, if you run the python script, it will download and merge the API schemas, but won't (re-) generate the code).

e-kononenko · 2024-10-23T11:57:40Z

tidalapi/bin/generate-api-files.py

+                        f"No lines starting with '{target_string}' found in file: {file_path}")
+
+
+def merge_openapi_schemas(file_paths, output_path):


Why was it required for you? Would a simpler approach (just process all files one by one and overwrite models) like we did on iOS not work here?

I feel like I answered this already, but the answer got lost.
In short: I generate interfaces for each place a oneOf item is used - and those places are distributed across all APIS. So if I generate them consecutively, each generation overwrites code I just created. That's why the generator needs full context before run.

tidalapi/src/main/kotlin/com/tidal/sdk/tidalapi/networking/AuthInterceptor.kt

tidalapi/src/main/kotlin/com/tidal/sdk/tidalapi/networking/DefaultAuthenticator.kt

tidalapi/bin/generate-api-files.py

stoyicker · 2024-10-23T13:39:18Z

I don't see why we would want to commit the generated code. It sounds more reasonable to me to generate it on CI on publish, put it in the published binary and that's it.

michpohl · 2024-10-24T04:35:06Z

I don't see why we would want to commit the generated code. It sounds more reasonable to me to generate it on CI on publish, put it in the published binary and that's it.

There are a number of reasons for it, here are some:

We want engineers to easily review the changes in generated code before merging
We want to keep open the ability to manually change details before merging, if needed
API code generation (especially on Android, where we needed to alter the generator) is still quite brittle, and the schemas are still in a phase where they might change quite significantly. This approach makes it easier to step in and apply quick fixes

I know that you added a similar comment on my earlier PR, and I would have liked to do it that way too. I did a good amount of work towards that goal. But this approach made the most sense to thew devs involved (not just for Android) and so we decided to go with this way.

However, once we are past an MVP situation and both schema design and client use have stabilized it might be nice to revisit the approach and maybe reconsider/improve, but currently this is the way to go.

This adds a python script that does the following: 1. Download the schemas 2. Join the schemas (assuming all entries with the same name are equal, which we discussed they should be. This works as intended now). 3. Run openapi-generator-cli from a specified location (since we will have to submit our own fork). Also added are the config yaml for the generator and the custom template for our ApiClient.

…yze mustache files The pattern matching would catch .kt.mustache before, now it doesn't anymore.

openapi-generator has a poat-processiong option, but since we're using a dedicated script to run ktlint, I decided to add the step for this here. With this change, ktlint will try and autoformat as many violations as possible. While not all of them get fixed, especially cleaning up imports is quite useful, as the generator creates a lot of unnecessary import statements.

This approach is taken from this tool: https://www.npmjs.com/package/openapi-merge-cli The tool itself didn't work out, but I like the approach over command line args, especially if its - like here - a number of long urls.

This ensures that in case of changed names/removals we don't end up with dead types that won't get removed.

michpohl force-pushed the michael/Set-up-API-client-code-generation branch from b93ab79 to 57e785c Compare October 23, 2024 05:32

michpohl requested a review from e-kononenko October 23, 2024 11:16

michpohl force-pushed the michael/Set-up-API-client-code-generation branch from 57e785c to 265c7fa Compare October 23, 2024 11:17

michpohl marked this pull request as ready for review October 23, 2024 11:17

michpohl requested a review from a team as a code owner October 23, 2024 11:17

michpohl commented Oct 23, 2024

View reviewed changes

michpohl enabled auto-merge October 23, 2024 11:39

e-kononenko reviewed Oct 23, 2024

View reviewed changes

e-kononenko approved these changes Oct 23, 2024

View reviewed changes

michpohl added 8 commits October 24, 2024 06:35

Add dependencies needed

345d7d1

Adjust run-ktlint.sh and the pre-commit hook to not accidentally anal…

0e26e2d

…yze mustache files The pattern matching would catch .kt.mustache before, now it doesn't anymore.

Add config json file to tell generate-api-files.py what to do

7405641

This approach is taken from this tool: https://www.npmjs.com/package/openapi-merge-cli The tool itself didn't work out, but I like the approach over command line args, especially if its - like here - a number of long urls.

Add generated files

bea26ad

Add networking classes to provide a configured Retrofit instance

6e1d3c2

Clear generated dir before generation

9573a62

This ensures that in case of changed names/removals we don't end up with dead types that won't get removed.

michpohl force-pushed the michael/Set-up-API-client-code-generation branch from 6d2c046 to 9573a62 Compare October 24, 2024 04:36

michpohl added this pull request to the merge queue Oct 24, 2024

ziad-halabi9 approved these changes Oct 24, 2024

View reviewed changes

Merged via the queue into main with commit 1f65455 Oct 24, 2024
7 checks passed

michpohl deleted the michael/Set-up-API-client-code-generation branch October 24, 2024 08:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set up API client code generation #121

Set up API client code generation #121

michpohl commented Oct 23, 2024 •

edited

Loading

michpohl Oct 23, 2024 •

edited

Loading

michpohl Oct 23, 2024 •

edited

Loading

e-kononenko Oct 23, 2024

michpohl Oct 24, 2024

stoyicker commented Oct 23, 2024

michpohl commented Oct 24, 2024 •

edited

Loading

		f"No lines starting with '{target_string}' found in file: {file_path}")


		def merge_openapi_schemas(file_paths, output_path):

Set up API client code generation #121

Set up API client code generation #121

Conversation

michpohl commented Oct 23, 2024 • edited Loading

Note for reviewers:

Note about the generated code:

How can you test this?

Do you need to test it to review this?

How to/ test code generation?

michpohl Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

michpohl Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

e-kononenko Oct 23, 2024

Choose a reason for hiding this comment

michpohl Oct 24, 2024

Choose a reason for hiding this comment

stoyicker commented Oct 23, 2024

michpohl commented Oct 24, 2024 • edited Loading

michpohl commented Oct 23, 2024 •

edited

Loading

michpohl Oct 23, 2024 •

edited

Loading

michpohl Oct 23, 2024 •

edited

Loading

michpohl commented Oct 24, 2024 •

edited

Loading